Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add a module not found error instead of assertion error #1

Merged
merged 10 commits into from
Apr 28, 2017
Merged

Add a module not found error instead of assertion error #1

merged 10 commits into from
Apr 28, 2017

Conversation

ranyitz
Copy link
Contributor

@ranyitz ranyitz commented Apr 25, 2017

If the module wasn't found, req-from is throwing an assertion error, this is because resolve-from is returning null in case the module wasn't found.

The PR will throw a module not found like error, with the same code and message.

another option would be to extend resolve-from in order to get the original error if that's a better solution.

@sindresorhus
Copy link
Owner

I would rather change resolve-from to be like this module. Throw on the main export and silent when using resolveFrom.silent().

@sindresorhus sindresorhus mentioned this pull request Apr 27, 2017
Merged
@ranyitz ranyitz mentioned this pull request Apr 27, 2017
Merged
@ranyitz
Copy link
Contributor Author

ranyitz commented Apr 27, 2017

I agree,

The changes are on this PR - sindresorhus/resolve-from#4

@sindresorhus
Copy link
Owner

resolve-from@3 is out. Can you update this PR?

@sindresorhus
Copy link
Owner

You also need to fix the merge conflict.

@ranyitz
Copy link
Contributor Author

ranyitz commented Apr 28, 2017

Updated, thanks for the quick response !

SamVerschueren
SamVerschueren suggested changes Apr 28, 2017
View reviewed changes
Copy link
Contributor

@SamVerschueren SamVerschueren left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you also rename fn to m in the tests?

test.js Outdated
@@ -1,9 +1,17 @@
import test from 'ava';
import fn from './';

test(t => {
test('reqFrom', t => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reqFrom()

test.js Outdated
t.is(moduleNotFoundError.message, 'Cannot find module \'./nonexistent\'');
});

test('reqFrom,silent', t => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reqFrom.silent()

@sindresorhus sindresorhus merged commit a197a73 into sindresorhus:master Apr 28, 2017
@sindresorhus
Copy link
Owner

Thank you @ranyitz :)

@ranyitz
Copy link
Contributor Author

ranyitz commented Apr 28, 2017

No problem, Thank you @sindresorhus @SamVerschueren :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SamVerschueren SamVerschueren SamVerschueren approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ranyitz @sindresorhus @SamVerschueren